-
-
Notifications
You must be signed in to change notification settings - Fork 603
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[16.0][FIX]pos_order_remove_line: Action on trash delete line #1120
Conversation
Hi @robyf70, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
pop @DorianMAG please correct the pre commit |
Dont understand why pre-commit failed. |
Finally understand, semicolon less in javascript file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this module is not concerned by the test failure.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/ocabot merge minor |
On my way to merge this fine PR! |
@robyf70 your merge command was aborted due to failed check(s), which you can inspect on this commit of 16.0-ocabot-merge-pr-1120-by-robyf70-bump-minor. After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red. |
@robyf70 |
No idea how to fix it |
@robyf70 |
@legalsylvain Can you please look at this error?
|
Let's try |
/ocabot merge minor |
What a great day to merge this nice PR. Let's do it! |
No time to investigate. indeed, something look broken in V16 branch. however, I just tested pos_order_to_sale_order solo, and it works. So I guess that it is an incompatibility with other module(s) |
@robyf70 your merge command was aborted due to failed check(s), which you can inspect on this commit of 16.0-ocabot-merge-pr-1120-by-robyf70-bump-minor. After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be merged as it is:
- The commits should be squashed into a single one
- The commit message should have a proper description, that contains a tag, the module name and the essence of the change e.g.: "[FIX] pos_order_remove_line: trash action"
- There are unnecessary changes in spacings
Hi @legalsylvain |
Thx, i take a look for commit recomandation. |
80aa31c
to
893fe4e
Compare
It's done, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Hi @DorianMAG, These changes revert some fixes made in #1044. I checked it on runboat, having
cc @papulo79 |
Hi @DorianMAG, I've repeat the process described by @danielduqma and I had the same results. This MR has broken the module with pos loyalty. |
@papulo79 @danielduqma |
@DorianMAG I take from the last comments that this PR is still not ready yet? |
Hi, but this one is ok |
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
Change javascript method to delete a line when click on the trash